New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Normalize HTTP lifecycle handlers with HTTP probers #86139
Normalize HTTP lifecycle handlers with HTTP probers #86139
Conversation
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Welcome @jasimmons! |
Hi @jasimmons. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
9b3088f
to
13eca1a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jasimmons !
thanks for your pr and welcome to kubernetes :)
Before reviewing the code in too much depth, I want to be sure I'm understanding the purpose of this pr :)
Can you share a little more about the specific bug(s) that we're trying to solve? Is it that we are not respecting headers passed to HTTPGetAction
.
Additionally, when you describe this diff as "normalizing", can you share more about what we are normalizing w.r.t? My initial thought was that other parts of k8s are using HttpDoer
, so we would be unifying behavior in this respect. However, I don't see any existing mentions of HttpDoer
.
/ok-to-test
Hey, @mattjmcnaughton , thanks! :)
That's part of it, yes. Also, though, lifecycle handlers always prepend a leading As an example, here's a pod: apiVersion: v1
kind: Pod
metadata:
name: sample-webserver
spec:
containers:
- name: webserver
image: alpine:3
command: ["/bin/sh"]
args: ["-c", "while true; do echo 'HTTP/1.1 204 No Content' | nc -l -p 80; done"]
ports:
- name: tcp-80
containerPort: 80
protocol: TCP
readinessProbe:
httpGet:
path: /ready
port: 80
scheme: HTTP
httpHeaders:
- name: foo
value: bar
lifecycle:
preStop:
httpGet:
path: /preStop
port: 80
scheme: HTTP
httpHeaders:
- name: foo
value: bar When applying this and viewing the logs of this pod, you can see the path is as-expected and HTTP headers are passed appropriately for the
However, when terminating this pod, the log shows that the path always has a
The normalization here is the behavior of that of The Thanks for the feedback! |
a426502
to
3bcf884
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha gotcha, ok that additional information is helpful - thank you :)
In my mind, there's a clear benefit to fixing the issues you outlined :) So its just a matter of finding the best way to do so.
One high level question I have - did we consider whether it would be possible to extract some functionality from pkg/probe/http/http.go
into a shared location, and then have both the lifecycle handlers and the http probers rely on it? Specifically some of the func here: https://github.com/kubernetes/kubernetes/blob/master/pkg/probe/http/http.go#L82.
I think part of deciding whether extracting common functionality would be wise is understanding if we're concerned about further drift between HTTP lifecycle handlers and HTTP probers in the future. Do you have thoughts on that topic?
Also, very curious to hear from others here :)
change lgtm, go ahead and squash down to the commits you want for merge and open a tracking issue for the two follow-ups (for the metric and the event) |
Happy to look over the metric once you have something ready. |
Align the behavior of HTTP-based lifecycle handlers and HTTP-based probers, converging on the probers implementation. This fixes multiple deficiencies in the current implementation of lifecycle handlers surrounding what functionality is available. The functionality is gated by the features.ConsistentHTTPGetHandlers feature gate.
5e5c61b
to
dfaaa14
Compare
I've squashed the changes down to two commits. No other changes were made since the last review. The follow up issue is #113174. |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: derekwaynecarr, jasimmons, liggitt, ohbus The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest |
@jasimmons: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
/test pull-kubernetes-e2e-gce-ubuntu-containerd |
Updated release note with event and metric name added in #113175 |
In the PR kubernetes#86139, two more lifecycle hook tests (poststart / prestop) were added using HTTPS. They are similar with the existing HTTP tests. However, this causes failures on Windows due to how networking works there. We previously fixed this in the HTTP tests via kubernetes@f9e4a01. This commit applies the same fix on the lifecycle hook HTTPS tests. Signed-off-by: Ionut Balutoiu <ibalutoiu@cloudbasesolutions.com>
In the PR kubernetes#86139, two more lifecycle hook tests (poststart / prestop) were added using HTTPS. They are similar with the existing HTTP tests. However, this causes failures on Windows due to how networking works there. We previously fixed this in the HTTP tests via kubernetes@f9e4a01. This commit applies the same fix on the lifecycle hook HTTPS tests. Signed-off-by: Ionut Balutoiu <ibalutoiu@cloudbasesolutions.com>
What type of PR is this?
/kind bug
What this PR does / why we need it:
Align the behavior of HTTP-based lifecycle handlers and HTTP-based probers, converging on the probers implementation. This fixes multiple deficiencies in the current implementation of lifecycle handlers surrounding what functionality is available.
The functionality is gated by the
features.ConsistentHTTPGetHandlers
feature gate.Which issue(s) this PR fixes:
Similar to #68846
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: